feat(core)!: implement write returns metadata #5562

meteorgan · 2025-01-19T09:24:56Z

Which issue does this PR close?

Part of #5557

Rationale for this change

What changes are included in this PR?

implement write returns metadata for Operator and BlockingOperator
enable write_has_content_length for all services and implement the logic for them
implement write returns metadata logic for service S3, fs, monoiofs, hdfs and webhdfs
other services return default Metadata after writing
skip return value for language bindings

Are there any user-facing changes?

Yes.

write, write_with in Operator returns Result<Metadata> instead of Return<()>
Writer.close() returns Return<Metadata> instead of Return<()>
write in BlockingOperator returns Return<Metadata> instead of Return<()>
FunctionWrite.call() used by BlockingOperator returns Return<Metadata> instead of Return<()>

meteorgan · 2025-01-19T15:22:19Z

emmm, another issue related to ceph rados: the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Xuanwo · 2025-01-20T03:40:47Z

the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Well, well, well...

Let's do our best to permit this since it's not harmful. Technically, we don't allow users to parse the ETag value; instead, they should store it and compare it with values from the same source. I'm guessing the issue lies in part of our code assuming that the ETag must contain "?

meteorgan · 2025-01-20T05:20:26Z

the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Well, well, well...

Let's do our best to permit this since it's not harmful. Technically, we don't allow users to parse the ETag value; instead, they should store it and compare it with values from the same source. I'm guessing the issue lies in part of our code assuming that the ETag must contain "?

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

Xuanwo · 2025-01-20T05:43:24Z

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

meteorgan · 2025-01-20T13:05:10Z

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

I've done some tests. it's very interesting that stat_with().if_match(etag) works fine regardless of whether the ETag includes the prefix and suffix " or not.

meteorgan · 2025-01-20T13:08:17Z

I've done some tests. it's very interesting that stat_with().if_match(etag) works fine regardless of whether the ETag includes the prefix and suffix " or not.

The results are the same across Ceph RADOS, Minio and S3

meteorgan · 2025-01-20T14:03:28Z

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

I still created a workaround by adding prefix and suffix " for Etag in the test if they are not included. I think this approach is simpler, what do you think ?

core/src/raw/adapters/kv/backend.rs

core/src/raw/adapters/typed_kv/backend.rs

core/src/raw/oio/write/append_write.rs

core/src/services/aliyun_drive/writer.rs

core/src/services/s3/writer.rs

core/tests/behavior/async_write.rs

Xuanwo · 2025-01-20T15:05:19Z

Perhaps we should introduce write_has_content_type flags to indicate which metadata is available in the meta returned by the write operation. Checking the meta.content_type() may pass the test but is meaningless to users; it could also potentially conceal incorrect implementations that developers can simplely return Metadata::default() to pass all tests. And it makes it more difficult for us to determine the implementation status of various services.

meteorgan · 2025-01-21T12:34:26Z

Perhaps we should introduce write_has_content_type flags to indicate which metadata is available in the meta returned by the write operation.

Would write_returns_content_type be a better name?

Checking the meta.content_type() may pass the test but is meaningless to users; it could also potentially conceal incorrect implementations that developers can simplely return Metadata::default() to pass all tests.

if a service returns Metadata::default(), it indicates that it implements nothing. There's a chance that a developer intended to return some contents but forgot set the fields. Unfortunately, the tests can't catch this.

And it makes it more difficult for us to determine the implementation status of various services.

Make sense ! Making it explicit is better.

meteorgan · 2025-01-21T12:46:06Z

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

Wow, they've already had a PR for this issue: ceph/ceph#60477

meteorgan · 2025-01-24T11:33:32Z

Gently push. Hi, @Xuanwo I've provided some replies, do you have any suggestions ?

Xuanwo · 2025-01-24T12:22:22Z

Would write_returns_content_type be a better name?

I prefer to align with exists stat_has_xxx and list_has_xxx.

Wow, they've already had a PR for this issue: ceph/ceph#60477

That's nice, maybe we can disable ceph test for now...

…e_returns_metadata logic for fs

meteorgan added services/s3 core breaking-changes labels Jan 19, 2025

github-actions bot added the releases-note/feat label Jan 19, 2025

meteorgan force-pushed the write_returns_metadata_for_s3 branch from 8591861 to 5c98d37 Compare January 19, 2025 10:44

meteorgan force-pushed the write_returns_metadata_for_s3 branch from 81aeb13 to ddc3de5 Compare January 20, 2025 13:47

meteorgan marked this pull request as ready for review January 20, 2025 14:03

meteorgan requested review from PsiACE, messense, Zheaoli, suyanhanx, tisonkun, silver-ymz and Xuanwo as code owners January 20, 2025 14:03

Xuanwo reviewed Jan 20, 2025

View reviewed changes

meteorgan force-pushed the write_returns_metadata_for_s3 branch from ddc3de5 to a1e0b0a Compare January 26, 2025 12:53

meteorgan changed the title ~~feat(core)!: implement write returns metadata for s3~~ feat(core)!: implement write returns metadata Jan 26, 2025

meteorgan mentioned this pull request Jan 26, 2025

Tracking Issue: Ceph RADOS returns invalid Etag when calling CompleteMultipartUpload #5575

Open

meteorgan added 11 commits January 27, 2025 19:22

feat(core): implement write returns metadata for s3

68cd8be

fix cargo clippy

73863a8

fix bindings: cpp, dotnet, haskell, ocaml, ruby

632eea9

fix bindings: java, nodejs, python

0ed4bc6

fix binding cpp

0bd0c96

fix test_retry_write_fail_on_close

d2b4f79

fix tests for Ceph RADOS and fix complete.rs

172fe15

set write_has_content_length as true by default && implement the writ…

40fc06a

…e_returns_metadata logic for fs

implement logic for monoiofs

c39303c

implement logic for webhdfs

634bdec

implement the logic for hdfs

d126087

meteorgan force-pushed the write_returns_metadata_for_s3 branch from 9482e08 to d126087 Compare January 27, 2025 11:31

return content_length for aliyun_drive after writing

2cad74d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core)!: implement write returns metadata #5562

feat(core)!: implement write returns metadata #5562

meteorgan commented Jan 19, 2025 •

edited

Loading

meteorgan commented Jan 19, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo commented Jan 20, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025

meteorgan commented Jan 24, 2025

Xuanwo commented Jan 24, 2025

feat(core)!: implement write returns metadata #5562

Are you sure you want to change the base?

feat(core)!: implement write returns metadata #5562

Conversation

meteorgan commented Jan 19, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

meteorgan commented Jan 19, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo commented Jan 20, 2025 • edited Loading

meteorgan commented Jan 21, 2025 • edited Loading

meteorgan commented Jan 21, 2025

meteorgan commented Jan 24, 2025

Xuanwo commented Jan 24, 2025

meteorgan commented Jan 19, 2025 •

edited

Loading

Xuanwo commented Jan 20, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025 •

edited

Loading